Allow string keys in `eval` utility #242

Raj-Parekh24 · 2023-03-11T13:52:24Z

Motivation for these changes

Closes #227

Implementation details

I have implemented a function that converts the dictionary with string keys to pytensor. The pytensor variable for the corresponding name is found using the function "get_var_by_name".

Checklist

Explain motivation and implementation 👆
Make sure that the pre-commit linting/style checks pass.
Link relevant issues, preferably in nice commit messages.
The commits correspond to relevant logical changes. Note that if they don't, we will rewrite/rebase/squash the git history before merging.
Are the changes covered by tests and docstrings?
Fill out the short summary sections 👇

Major / Breaking Changes

...

New features

... Its and enhancement of current functionality eval present in graph/basic.py under Variable class. Through this enhancement eval will accept string i.e. pytensor node name.

Bugfixes

...

Documentation

...

Maintenance

...

ricardoV94

This looks great @Raj-Parekh24

I left some suggestions below

tests/graph/test_basic.py

Raj-Parekh24 · 2023-03-11T18:23:58Z

While running pre-commit run --all in the local machine I am getting a series of errors with the last message as 1 files unexpectedly failed. pytensor/link/c/exceptions.py These files did not fail before, so please check the above output for errors in {'pytensor/link/c/exceptions.py'} and fix them. You can run python scripts/run_mypy.py --verbose to reproduce this test locally.

When I check its logs for basic.py there are no errors from my changes.

Is this expected behavior or I have broken something?

I am using python 3.11

…are defined

Raj-Parekh24 · 2023-03-11T18:41:03Z

And in the last build, the test case failed for multiple environments is:
FAILED tests/link/jax/test_scalar.py::test_erfinv - AssertionError:

ricardoV94 · 2023-03-11T21:10:50Z

And in the last build, the test case failed for multiple environments is:
FAILED tests/link/jax/test_scalar.py::test_erfinv - AssertionError:

Yeah needs to be investigated. Probably a new release of jax that behaves differently.

The tracking issue is #241

ricardoV94 · 2023-03-11T21:11:33Z

Mypy doesn't work on python 3.11, issue here #240

Raj-Parekh24 · 2023-03-12T04:01:12Z

Thanks for the help @ricardoV94 ,

I have updated the code and added warning when multiple variables are there for same name.

And also updated the test cases accordingly.

Please review it.

pytensor/graph/basic.py

ricardoV94 · 2023-03-13T14:28:05Z

pytensor/graph/basic.py

+                        warnings.warn(
+                            f"Found {length_of_nodes_with_matching_names} pytensor variables with name {i} taking the first declared named variable for computation"
+                        )


It seems safer to just fail.

Instead of warning will throw Exception instead.

ricardoV94 · 2023-03-13T14:28:46Z

pytensor/graph/basic.py

+        >>> import numpy as np
+        >>> import pytensor.tensor as at
+        >>> x = at.dscalar('x')
+        >>> y = at.dscalar('y')
+        >>> z = x + y
+        >>> np.allclose(z.eval({'x' : 3, 'y' : 1}), 4)
+        True


This does not show a use of the function. In any case we don't need a docstring with an example I think

Since we are making it internal to eval I also think that the doc-string will be no more required.

ricardoV94 · 2023-03-13T14:29:55Z

pytensor/graph/basic.py

@@ -558,6 +558,46 @@ def get_parents(self):
            return [self.owner]
        return []

+    def convert_string_keys_to_pytensor_variables(self, inputs_to_values):


Given we are in pytensor, no need to mention it in the name. If you want you can specify the return type is Dict[Variable, Variable]

Suggested change

def convert_string_keys_to_pytensor_variables(self, inputs_to_values):

def convert_string_keys_to_variables(self, inputs_to_values):

sounds good, will replace the function name with suggested one.

ricardoV94 · 2023-03-13T14:31:27Z

tests/graph/test_basic.py

+        assert self.w.eval({self.z: 3}) == 6.0
+
+    def test_eval_with_strings_with_mulitple_same_name(self):
+        assert self.t.eval({"e": 1.0}) == 2.0


I would create the new variables here instead of setup_method. It's arguably more readable and these are unlikely to be used in other tests

ok, instead of creating variables in setup_method will add the variables in test_function.

ricardoV94 · 2023-03-13T14:32:15Z

@Raj-Parekh24 nice work. I left some comments above. Let me know what you think.

Raj-Parekh24 · 2023-03-14T12:46:12Z

Hi @ricardoV94 ,

Thanks for the review and the suggestions, I have updated the implementation accordingly and pushed a new commit regarding it. Please review it and suggest the required changes.

Raj-Parekh24 · 2023-03-16T16:09:40Z

Hey @ricardoV94 ,

I have updated the code bases on your suggestions.

And I also want to work on issue #55, I got the requirements but still not able to understand how to implement it. Since I am new to the repo, can you please give me starting steps to work on it.

ricardoV94 · 2023-03-16T19:52:19Z

Hi @Raj-Parekh24 sorry for the delay. I'll try to review your PR tomorrow. For that other PR I will give some more pointers there

Thanks for all the work!

Raj-Parekh24 · 2023-03-17T02:10:33Z

Thanks for all the help and support @ricardoV94 .

ricardoV94

Looks good. I left some suggestions below

ricardoV94 · 2023-03-17T19:59:50Z

pytensor/graph/basic.py

+                            nodes_with_matching_names[
+                                length_of_nodes_with_matching_names - 1
+                            ]


This can just be nodes_with_matching_names[0], now that you checked only 1 case exists.

Thank you for your review @ricardoV94. Will update the index value.

tests/graph/test_basic.py

…able

Raj-Parekh24 · 2023-03-18T14:39:08Z

Thanks a lot for your review @ricardoV94,

I have added the code based on what you suggested. Please review it and kindly give your suggestions.

ricardoV94

Functionality-wise this PR is ready! I just left some comments on code formatting. Let me know if you have any questions.

ricardoV94 · 2023-03-20T10:56:51Z

tests/graph/test_basic.py

+        assert self.w.eval({"x": 1.0, self.y: 2.0}) == 6.0
+        assert self.w.eval({self.z: 3}) == 6.0
+
+    def test_eval_errors_having_mulitple_variables_same_name(self):


Suggested change

def test_eval_errors_having_mulitple_variables_same_name(self):

def test_eval_with_strings_multiple_matches(self):

ricardoV94 · 2023-03-20T10:57:04Z

tests/graph/test_basic.py

+        with pytest.raises(Exception, match="Found 2 pytensor variables with name e"):
+            t.eval({"e": 1})
+
+    def test_eval_errors_with_no_name_exists(self):


Suggested change

def test_eval_errors_with_no_name_exists(self):

def test_eval_with_strings_no_match(self):

ricardoV94 · 2023-03-20T11:03:17Z

pytensor/graph/basic.py

+        def convert_string_keys_to_variables():
+            process_input_to_values = {}
+            for i in inputs_to_values:
+                if isinstance(i, str):
+                    nodes_with_matching_names = get_var_by_name([self], i)
+                    length_of_nodes_with_matching_names = len(nodes_with_matching_names)
+                    if length_of_nodes_with_matching_names == 0:
+                        raise Exception(f"{i} not found in graph")
+                    else:
+                        if length_of_nodes_with_matching_names > 1:
+                            raise Exception(
+                                f"Found {length_of_nodes_with_matching_names} pytensor variables with name {i}"
+                            )
+                        process_input_to_values[
+                            nodes_with_matching_names[0]
+                        ] = inputs_to_values[i]
+                else:
+                    process_input_to_values[i] = inputs_to_values[i]
+            return process_input_to_values
+
+        inputs_to_values = convert_string_keys_to_variables()


It should be a bit more "pythonic" like this (I did not test the code!):

Suggested change

def convert_string_keys_to_variables():

process_input_to_values = {}

for i in inputs_to_values:

if isinstance(i, str):

nodes_with_matching_names = get_var_by_name([self], i)

length_of_nodes_with_matching_names = len(nodes_with_matching_names)

if length_of_nodes_with_matching_names == 0:

raise Exception(f"{i} not found in graph")

else:

if length_of_nodes_with_matching_names > 1:

raise Exception(

f"Found {length_of_nodes_with_matching_names} pytensor variables with name {i}"

)

process_input_to_values[

nodes_with_matching_names[0]

] = inputs_to_values[i]

else:

process_input_to_values[i] = inputs_to_values[i]

return process_input_to_values

inputs_to_values = convert_string_keys_to_variables()

def convert_string_keys_to_variables(input_to_values):

new_input_to_values = {}

for key, value in inputs_to_values.items():

if isinstance(key, str):

matching_vars = get_var_by_name([self], key)

if not matching_vars:

raise Exception(f"{key} not found in graph")

elif len(matching_vars) > 1:

raise Exception(

f"Found multiple variables with name {key}"

)

input_to_values[matching_vars[0]] = value

else:

new_input_to_values[key] = value

return new_input_to_values

inputs_to_values = convert_string_keys_to_variables(inputs_to_values)

Raj-Parekh24 · 2023-03-20T15:35:47Z

Thanks for the review @ricardoV94 ,

I have incorporated the suggested changes in the new commit, please review it.

I also wanted to implement a feature under issue #55, the initial points can help me to make things clear.

ricardoV94

Looks good, running the tests now!

codecov-commenter · 2023-03-22T08:26:33Z

Codecov Report

Merging #242 (9008a32) into main (0c9eb9c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #242   +/-   ##
=======================================
  Coverage   80.44%   80.44%           
=======================================
  Files         170      170           
  Lines       45328    45341   +13     
  Branches    11069    11073    +4     
=======================================
+ Hits        36463    36476   +13     
  Misses       6642     6642           
  Partials     2223     2223

Impacted Files	Coverage Δ
pytensor/graph/basic.py	`88.90% <100.00%> (+0.20%)`	⬆️

Added functionality that can use name of variable in eval

b088b01

ricardoV94 added the enhancement New feature or request label Mar 11, 2023

ricardoV94 changed the title ~~Added functionality that can use name of variable in eval~~ Allow string keys in eval utility Mar 11, 2023

ricardoV94 reviewed Mar 11, 2023

View reviewed changes

tests/graph/test_basic.py Outdated Show resolved Hide resolved

Updated function with warning when multiple variables with same name …

8976c94

…are defined

ricardoV94 reviewed Mar 13, 2023

View reviewed changes

Making convert_string_keys_to_variables internal to eval

b7f745e

ricardoV94 reviewed Mar 17, 2023

View reviewed changes

Removed last variable selection and added test case for no name avail…

8869a9d

…able

ricardoV94 requested changes Mar 20, 2023

View reviewed changes

Raj-Parekh24 added 3 commits March 20, 2023 20:48

Merge branch 'main' into issue-227

d889182

Modified the code structure based on suggestions

6e2efaa

Modified the code structure based on suggestions

9008a32

ricardoV94 approved these changes Mar 22, 2023

View reviewed changes

ricardoV94 merged commit feccc41 into pymc-devs:main Mar 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow string keys in `eval` utility #242

Allow string keys in `eval` utility #242

Raj-Parekh24 commented Mar 11, 2023

ricardoV94 left a comment

Raj-Parekh24 commented Mar 11, 2023 •

edited

Loading

Raj-Parekh24 commented Mar 11, 2023

ricardoV94 commented Mar 11, 2023

ricardoV94 commented Mar 11, 2023

Raj-Parekh24 commented Mar 12, 2023

ricardoV94 Mar 13, 2023

Raj-Parekh24 Mar 14, 2023

ricardoV94 Mar 13, 2023

Raj-Parekh24 Mar 14, 2023

ricardoV94 Mar 13, 2023

Raj-Parekh24 Mar 14, 2023

ricardoV94 Mar 13, 2023

Raj-Parekh24 Mar 14, 2023

ricardoV94 commented Mar 13, 2023

Raj-Parekh24 commented Mar 14, 2023

Raj-Parekh24 commented Mar 16, 2023

ricardoV94 commented Mar 16, 2023

Raj-Parekh24 commented Mar 17, 2023

ricardoV94 left a comment

ricardoV94 Mar 17, 2023

Raj-Parekh24 Mar 18, 2023

Raj-Parekh24 commented Mar 18, 2023

ricardoV94 left a comment

ricardoV94 Mar 20, 2023

ricardoV94 Mar 20, 2023

ricardoV94 Mar 20, 2023

Raj-Parekh24 commented Mar 20, 2023 •

edited

Loading

ricardoV94 left a comment

codecov-commenter commented Mar 22, 2023

	def convert_string_keys_to_pytensor_variables(self, inputs_to_values):
	def convert_string_keys_to_variables(self, inputs_to_values):

	def test_eval_errors_having_mulitple_variables_same_name(self):
	def test_eval_with_strings_multiple_matches(self):

	def test_eval_errors_with_no_name_exists(self):
	def test_eval_with_strings_no_match(self):

Allow string keys in eval utility #242

Allow string keys in eval utility #242

Conversation

Raj-Parekh24 commented Mar 11, 2023

Motivation for these changes

Implementation details

Checklist

Major / Breaking Changes

New features

Bugfixes

Documentation

Maintenance

ricardoV94 left a comment

Choose a reason for hiding this comment

Raj-Parekh24 commented Mar 11, 2023 • edited Loading

Raj-Parekh24 commented Mar 11, 2023

ricardoV94 commented Mar 11, 2023

ricardoV94 commented Mar 11, 2023

Raj-Parekh24 commented Mar 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricardoV94 commented Mar 13, 2023

Raj-Parekh24 commented Mar 14, 2023

Raj-Parekh24 commented Mar 16, 2023

ricardoV94 commented Mar 16, 2023

Raj-Parekh24 commented Mar 17, 2023

ricardoV94 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Raj-Parekh24 commented Mar 18, 2023

ricardoV94 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Raj-Parekh24 commented Mar 20, 2023 • edited Loading

ricardoV94 left a comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 22, 2023

Codecov Report

Allow string keys in `eval` utility #242

Allow string keys in `eval` utility #242

Raj-Parekh24 commented Mar 11, 2023 •

edited

Loading

Raj-Parekh24 commented Mar 20, 2023 •

edited

Loading